[FLINK-36760][sql-client] Supports to deploy script via sql client #25754

fsk119 · 2024-12-06T12:13:21Z

What is the purpose of the change

Support to deploy script via SQL Client.

Brief change log

Support to deploy script
Support to deploy script with remote jar
Relax the limitation that script exists in local

Verifying this change

This change added tests and can be verified as follows:

Added integration tests to check deploy script with fake target
Added E2E case to fetch resources from the remote jar

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2024-12-06T12:17:16Z

CI report:

c459f44 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

davidradl · 2024-12-13T14:50:12Z

Reviewed by Chi on 12/12/24 Group to test and/or review outside of the meeting. @tomncooper one you might want to look at.

hackergin

@fsk119 Thanks for your contribution, only some minor comments.

...able/flink-sql-jdbc-driver/src/test/java/org/apache/flink/table/jdbc/FlinkStatementTest.java

flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java

hackergin

@fsk119 Thanks for the update, LGTM.

...table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliOptionsParser.java

hackergin · 2025-01-07T03:21:39Z

flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java

+                String scheme = StringUtils.lowerCase(uri.getScheme());
+                String clusterId;
+                // local files
+                if (scheme == null || scheme.equals("file")) {


Would it be better if we could support shipping local files to the remote? That said, I think the current implementation is also fine.

Here we just read the sql script file if the file is in local fs. It's not an easy work to ship files in the k8s deployment. It requires users to configure a remote oss env.

lsyldliu

@fsk119 Thanks for your contribution, LGTM overall, just left some minor comments.

lsyldliu · 2025-01-07T03:19:40Z

flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliOptions.java

@@ -35,16 +36,16 @@ public class CliOptions {

    private final boolean isPrintHelp;
    private final String sessionId;
-    private final URL initFile;
-    private final URL sqlFile;
+    private final URI initFile;


I'm just a little curious that why we change it to URI type?

Because URL requires to qualify referred resources. For example, if users uses the hdfs://path/to/object as the input, the sql client needs to validate whether the protocol is valid. However, in the client side, users may not configure hdfs parameters.

lsyldliu · 2025-01-07T03:28:43Z

flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java

@@ -213,6 +213,8 @@ public AttributedString build() {

    public static final String MESSAGE_EXECUTE_STATEMENT = "Execute statement succeeded.";

+    public static final String MESSAGE_DEPLOY_SCRIPT = "Deploy script in application mode: ";


"Deploy SQL script in application mode: "?
BTW, the content after the colon is an SQL script?

It's better to align the style with other prompt message(we only upper case the first word), you can take a look at the line above.

The content after the colon is the cluster id, which helps users to find the deployment

lsyldliu · 2025-01-07T05:41:15Z

...table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/ExecutorImpl.java

@@ -331,6 +334,19 @@ public List<String> completeStatement(String statement, int position) {
                .getCandidates();
    }

+    @Override
+    public String deployScript(@Nullable String script, @Nullable URI uri) {
+        return getResponse(


I think it would better that we check the two parameters can not be null simutaneously.

I delay the check in the server side.

lsyldliu · 2025-01-07T05:52:14Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterClientFactory.java

@@ -78,6 +80,12 @@ private YarnClusterDescriptor getClusterDescriptor(Configuration configuration)
        final YarnConfiguration yarnConfiguration =
                Utils.getYarnAndHadoopConfiguration(configuration);

+        if (System.getenv().get("IN_TESTS") != null) {


The aim of this change is?

In Integration test setup, the Yarn containers created by YarnMiniCluster does not have the Yarn site XML and KRB5 configuration files. We are adding these files as container local resources for the container applications (JM/TMs) to have proper secure cluster setup

lsyldliu · 2025-01-07T06:03:47Z

flink-yarn-tests/src/test/java/org/apache/flink/yarn/SqlYARNApplicationITCase.java

+                        + ") WITH (\n"
+                        + "  'connector' = 'blackhole'\n"
+                        + ");\n"
+                        + "INSERT INTO sink VALUES (1), (2), (3);");


Can we add test to cover the udf jar?

Because code freeze time is coming, I will add a test in another PR.

lsyldliu · 2025-01-07T06:29:09Z

flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/SqlClient.java

+                    throw new SqlClientException(
+                            "Sql Client doesn't support to run init files when deploying script into cluster.");
+                }
+                boolean success = cli.executeInitialization(options.getInitFile());


I'm confuse that what content are init file? is it sql query, sql ddl or config option? After see the line 147 and line 153 error message, I guess it is a sql query.

You can take a look at th doc.

Script file that used to init the session context. If ge error in execution, the sql client will exit. Notice it's not allowed to add query or insert into the init file.

lsyldliu

LGTM

…pache#25754)

flinkbot added the component=TableSQL/Client label Dec 6, 2024

hackergin reviewed Dec 14, 2024

View reviewed changes

...able/flink-sql-jdbc-driver/src/test/java/org/apache/flink/table/jdbc/FlinkStatementTest.java Outdated Show resolved Hide resolved

flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java Outdated Show resolved Hide resolved

fsk119 force-pushed the client branch 5 times, most recently from 5d1f6dc to 36cd7bc Compare January 4, 2025 15:25

hackergin approved these changes Jan 7, 2025

View reviewed changes

fsk119 added 6 commits January 7, 2025 12:03

[FLINK-36760][sql-client] Supports to deploy script via sql client

6fdd1e6

address comments

9959618

add yarn e2e case

4244fcf

fix failed test

23e5361

fix failed tests and improve codes

09c797f

address comments

c459f44

fsk119 force-pushed the client branch from 22a79e3 to c459f44 Compare January 7, 2025 04:04

lsyldliu reviewed Jan 7, 2025

View reviewed changes

lsyldliu approved these changes Jan 7, 2025

View reviewed changes

fsk119 merged commit 4a8b8c0 into apache:master Jan 7, 2025

xaniasd pushed a commit to xaniasd/flink that referenced this pull request Jan 13, 2025

[FLINK-36760][sql-client] Supports to deploy script via sql client (a…

a9c0450

…pache#25754)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-36760][sql-client] Supports to deploy script via sql client #25754

[FLINK-36760][sql-client] Supports to deploy script via sql client #25754

fsk119 commented Dec 6, 2024 •

edited

Loading

flinkbot commented Dec 6, 2024 •

edited

Loading

davidradl commented Dec 13, 2024

hackergin left a comment

hackergin left a comment

hackergin Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu left a comment

lsyldliu Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu Jan 7, 2025

fsk119 Jan 7, 2025

lsyldliu left a comment

		@@ -213,6 +213,8 @@ public AttributedString build() {

		public static final String MESSAGE_EXECUTE_STATEMENT = "Execute statement succeeded.";

		public static final String MESSAGE_DEPLOY_SCRIPT = "Deploy script in application mode: ";

[FLINK-36760][sql-client] Supports to deploy script via sql client #25754

[FLINK-36760][sql-client] Supports to deploy script via sql client #25754

Conversation

fsk119 commented Dec 6, 2024 • edited Loading

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Dec 6, 2024 • edited Loading

CI report:

davidradl commented Dec 13, 2024

hackergin left a comment

Choose a reason for hiding this comment

hackergin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsyldliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsyldliu left a comment

Choose a reason for hiding this comment

fsk119 commented Dec 6, 2024 •

edited

Loading

flinkbot commented Dec 6, 2024 •

edited

Loading